Conversation
+ Switch to source-generated Locale with binding support + Move Locale class from Hi3Helper.Core to CollapseLauncher project scope
neon-nyan
added
Enhancement
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
+90
to
+100
| return 0; | ||
| } | ||
|
|
||
| // Get first value reference | ||
| TKey? firstKey = dict.Keys.FirstOrDefault(); | ||
| if (firstKey == null) | ||
| { | ||
| throw new InvalidOperationException(); | ||
| } | ||
| ref TValue firstRef = ref CollectionsMarshal.GetValueRefOrNullRef(dict, firstKey); | ||
| if (Unsafe.IsNullRef(ref firstRef)) |
There was a problem hiding this comment.
Bug: The GetValueIndexFromKey method uses unsafe pointer arithmetic to find a dictionary value's index, which is unreliable and not guaranteed to work by the .NET runtime.
Severity: CRITICAL
Suggested Fix
Replace the unsafe pointer arithmetic in GetValueIndexFromKey. A reliable approach would be to convert the dictionary's keys or values to a list and use the IndexOf() method to find the correct index. For example: dict.Keys.ToList().IndexOf(key).
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
CollapseLauncher/Classes/Helper/Metadata/LauncherMetadataHelper.GameTitleAndRegion.cs#L83-L100
Potential issue: The `GetValueIndexFromKey` method calculates a dictionary value's index
using unsafe pointer arithmetic. It computes the byte offset between the first value's
reference and the target value's reference, then divides by a hardcoded or calculated
size. This approach incorrectly assumes that a .NET `Dictionary<TKey, TValue>` stores
its values in a contiguous memory block ordered by insertion, which is not a guaranteed
contract. This faulty index is then used to select an item in a UI combo box, leading to
the wrong game region being displayed and potentially used, and also causes unnecessary
re-initialization of Discord's Rich Presence feature due to incorrect equality checks.
Did we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| // Initialize and clear the stamp dictionary | ||
| LauncherMetadataStampDictionary ??= new Dictionary<string, Stamp>(); | ||
| LauncherMetadataStampDictionary.Clear(); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| HttpClient client = FallbackCDNUtil.GetGlobalHttpClient(true); | ||
| using FileStream fileStream = localeFileInfo.Create(); | ||
| using Stream httpStream = client.GetStreamAsync(localeUrl1).Result; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Main Goal
The localization has been rewritten entirely to enable couple of improvements for easier UI element maintainability and binding process. List of improvements in this PR as follows:
[New] Switching to source-generated JSON -> Class mapper & deserializer.
Previously, developer should map the property inside of the source-code to represents entries inside of the locale
.jsonfile. Thanks to source-generated mapper, the property will be populated and generated automatically each time new entries have been added to the.jsonfile.[New] Add binding support to the Locale object.
This enables each UI Elements that binds their represented Locale property, to be able to have its text updated while a locale is being changed to another language without the needs of manual binding update.
PR Status :
Templates
Changelog Prefixes